feat(opentelemetry): create otel instrumentation for typed-express-router #1044
Conversation
starfy84
force-pushed
the
DX-1473-create-otel-wrapper-for-typed-express-router
branch
from
7b37e6f to
9c177cd
Compare
typed-express-router
starfy84
force-pushed
the
DX-1473-create-otel-wrapper-for-typed-express-router
branch
from
9c177cd to
0bff27e
Compare
packages/opentelemetry-instrumentation-typed-express-router/test/instrumentation.test.ts
Outdated
Show resolved
Hide resolved
| // Note: We cannot test the actual patching functionality in isolation | ||
| // because it requires a real typed-express-router module. | ||
| // Integration tests will cover this part. |
There was a problem hiding this comment.
Can you help me understand the utility provided by this test file? I see that we are checking whether or not internal values are set, but we aren't extending the assertion to cover the behavior that this modifies in an TypedExpressRouterInstrumentation instance.
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-typed-express-router/src/version.ts
Outdated
Show resolved
Hide resolved
| export function handleValidationErrors(span: Span, errors: Errors): void { | ||
| const validationErrors = PathReporter.failure(errors); | ||
| const validationErrorMessage = validationErrors.join('\n'); | ||
|
|
||
| span.setStatus({ | ||
| code: SpanStatusCode.ERROR, | ||
| message: 'Validation error', | ||
| }); | ||
|
|
||
| span.setAttribute(ApiTsAttributes.API_TS_VALIDATION_ERROR, validationErrorMessage); | ||
|
|
||
| // Record exception event for better error reporting | ||
| span.recordException({ | ||
| name: 'ValidationError', | ||
| message: validationErrorMessage, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Code coverage in this package is at 32% of statements, so I'm not sure we've sufficiently asserted that the instrumentation use cases we expose create the desired spans. This function isn't used anywhere else in this PR. Can you please create a test case for it and other functions exposed to users of this library?
We should include assertions that fix the behavior of this code, namely that this instrumenter creates spans containing the data and format we desire. This prevents future regressions and acts as a form of documentation because users of the library can be the test cases to learn from a known working context how to use this library and what benefit it provides them.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| SimpleSpanProcessor, | ||
| } from '@opentelemetry/sdk-trace-base'; | ||
| import { AsyncLocalStorageContextManager } from '@opentelemetry/context-async-hooks'; | ||
| import { SpanStatusCode, SpanKind, trace, context, Span } from '@opentelemetry/api'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the issue, the unused SpanStatusCode import should be removed from the import statement on line 14. This will eliminate unnecessary clutter and improve code readability. No additional changes are required, as removing the unused import does not affect the functionality of the code.
| @@ -13,3 +13,3 @@ | ||
| import { AsyncLocalStorageContextManager } from '@opentelemetry/context-async-hooks'; | ||
| import { SpanStatusCode, SpanKind, trace, context, Span } from '@opentelemetry/api'; | ||
| import { SpanKind, trace, context, Span } from '@opentelemetry/api'; | ||
| import { afterEach, before, beforeEach, describe, test } from 'node:test'; |
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
| import { apiSpec, httpRequest, httpRoute, optional } from '@api-ts/io-ts-http'; | ||
| import { TypedRequestHandler, createRouter } from '@api-ts/typed-express-router'; | ||
| import { ApiTsAttributes } from '../src/utils'; | ||
| import { handleGenericError, handleValidationErrors } from '../src/utils'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the issue, the unused imports handleGenericError and handleValidationErrors should be removed from the file. This will clean up the code and eliminate unnecessary dependencies. The changes should be made directly to the import statement on line 27.
| @@ -26,3 +26,3 @@ | ||
| import { TypedRequestHandler, createRouter } from '@api-ts/typed-express-router'; | ||
| import { handleGenericError, handleValidationErrors } from '../src/utils'; | ||
|
|
||
| import { Errors } from 'io-ts'; |
| import { TypedRequestHandler, createRouter } from '@api-ts/typed-express-router'; | ||
| import { ApiTsAttributes } from '../src/utils'; | ||
| import { handleGenericError, handleValidationErrors } from '../src/utils'; | ||
| import { Errors } from 'io-ts'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the problem, the unused import Errors should be removed from the file. This will improve code readability and eliminate unnecessary clutter. The removal of the import does not affect the functionality of the code, as Errors is not referenced anywhere in the provided snippet.
| @@ -27,3 +27,3 @@ | ||
| import { handleGenericError, handleValidationErrors } from '../src/utils'; | ||
| import { Errors } from 'io-ts'; | ||
|
|
||
| import { makeRequest } from './util'; |
packages/opentelemetry-instrumentation-typed-express-router/test/integration.test.ts
Fixed
Show fixed
Hide fixed
starfy84
force-pushed
the
DX-1473-create-otel-wrapper-for-typed-express-router
branch
from
43daf20 to
3781856
Compare
|
|
||
| // Register instrumentation before importing modules | ||
| import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; | ||
| import { RPCType, setRPCMetadata } from '@opentelemetry/core'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the problem, the unused imports RPCType and setRPCMetadata should be removed from the file. This will eliminate unnecessary clutter and improve code readability. The change involves editing the import statement on line 7 to exclude these unused elements.
| @@ -6,3 +6,3 @@ | ||
| import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; | ||
| import { RPCType, setRPCMetadata } from '@opentelemetry/core'; | ||
| import {} from '@opentelemetry/core'; | ||
| import { TypedExpressRouterInstrumentation } from '../src'; |
| import express from 'express'; | ||
| import * as http from 'http'; | ||
| import * as t from 'io-ts'; | ||
| import { apiSpec, httpRequest, httpRoute, optional } from '@api-ts/io-ts-http'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the issue, the unused optional import should be removed from the import statement on line 20. This will clean up the code and eliminate the unnecessary import. No additional changes are required since removing an unused import does not affect the functionality of the code.
| @@ -19,3 +19,3 @@ | ||
| import * as t from 'io-ts'; | ||
| import { apiSpec, httpRequest, httpRoute, optional } from '@api-ts/io-ts-http'; | ||
| import { apiSpec, httpRequest, httpRoute } from '@api-ts/io-ts-http'; | ||
|
|
| // Unpatch if needed | ||
| if ( | ||
| typeof moduleExports.wrapRouter === 'function' && | ||
| (moduleExports.wrapRouter as any)[kPatched] |
Check warning
Code scanning / CodeQL
Implicit operand conversion Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 17 hours ago
To fix the issue, we need to ensure that kPatched is explicitly checked or converted to the correct type before being used as a property key. The best approach is to validate that kPatched is defined and of the expected type (e.g., string or symbol) before accessing it. If kPatched is undefined, the code should handle this case explicitly to avoid runtime errors or unintended behavior.
The fix involves:
- Adding a type check for
kPatchedto ensure it is defined and of the correct type. - Using explicit logic to handle cases where
kPatchedis undefined or invalid.
| @@ -47,2 +47,3 @@ | ||
| typeof moduleExports.wrapRouter === 'function' && | ||
| kPatched !== undefined && | ||
| (moduleExports.wrapRouter as any)[kPatched] | ||
| @@ -57,2 +58,3 @@ | ||
| typeof moduleExports.createRouter === 'function' && | ||
| kPatched !== undefined && | ||
| (moduleExports.createRouter as any)[kPatched] |
|
|
||
| if ( | ||
| typeof moduleExports.createRouter === 'function' && | ||
| (moduleExports.createRouter as any)[kPatched] |
Check warning
Code scanning / CodeQL
Implicit operand conversion Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 17 hours ago
To fix the issue, we need to ensure that kPatched is explicitly defined and used correctly. If kPatched is intended to be a string or symbol, we should verify its definition and ensure it is not undefined. If it is undefined, we should provide a default value or throw an error to prevent implicit conversion.
The best way to fix this is to:
- Verify that
kPatchedis properly imported and initialized in the./typesmodule. - Explicitly check for
kPatched's validity before using it as a property key. - If
kPatchedisundefined, provide a fallback value or handle the error explicitly.
| @@ -57,2 +57,3 @@ | ||
| typeof moduleExports.createRouter === 'function' && | ||
| kPatched !== undefined && | ||
| (moduleExports.createRouter as any)[kPatched] |
| return original.apply(this, args); | ||
| }; | ||
|
|
||
| (patched as any)[kPatched] = true; |
Check warning
Code scanning / CodeQL
Implicit operand conversion Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 17 hours ago
To fix the issue, we should ensure that kPatched is explicitly defined and has a valid value before it is used as a key in the expression (patched as any)[kPatched] = true;. This can be achieved by adding a fallback value (e.g., a default string) in case kPatched is undefined. This approach ensures that the code behaves predictably and avoids implicit conversions.
The fix involves modifying the line where kPatched is used to include a fallback value. For example, we can use the nullish coalescing operator (??) to provide a default string key if kPatched is undefined.
| @@ -81,3 +81,3 @@ | ||
|
|
||
| (patched as any)[kPatched] = true; | ||
| (patched as any)[kPatched ?? 'patched'] = true; | ||
| (patched as any).__original = original; |
| context.active(), | ||
| (decodeSpan) => { | ||
| // Add attributes to the span | ||
| decodeSpan.setAttribute(ApiTsAttributes.API_TS_ROUTE_NAME, apiName); |
Check failure
Code scanning / CodeQL
Property access on null or undefined Error
Copilot Autofix
AI about 17 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| decodeSpan.setAttribute(ApiTsAttributes.API_TS_ROUTE_NAME, apiName); | ||
| if (req.httpRoute) { | ||
| decodeSpan.setAttribute( | ||
| ApiTsAttributes.API_TS_ROUTE_PATH, |
Check failure
Code scanning / CodeQL
Property access on null or undefined Error
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 17 hours ago
To fix the issue, we need to ensure that ApiTsAttributes.API_TS_ROUTE_PATH is properly defined and initialized before accessing it. If it is not defined, we should provide a fallback value to prevent runtime errors. This can be achieved by using optional chaining (?.) or a default value (|| '') when accessing the property.
Specifically:
- Update the code on line 120 to use a fallback value (
|| '') forApiTsAttributes.API_TS_ROUTE_PATH. - Similarly, update other instances where properties of
ApiTsAttributesare accessed, such as on line 125, to ensure robustness.
No additional methods, imports, or definitions are required for this fix.
| @@ -119,8 +119,8 @@ | ||
| decodeSpan.setAttribute( | ||
| ApiTsAttributes.API_TS_ROUTE_PATH, | ||
| req.httpRoute.path || '', | ||
| ApiTsAttributes?.API_TS_ROUTE_PATH || '', | ||
| req.httpRoute?.path || '', | ||
| ); | ||
| decodeSpan.setAttribute( | ||
| ApiTsAttributes.API_TS_ROUTE_METHOD, | ||
| req.httpRoute.method || '', | ||
| ApiTsAttributes?.API_TS_ROUTE_METHOD || '', | ||
| req.httpRoute?.method || '', | ||
| ); | ||
| @@ -144,8 +144,8 @@ | ||
| encodeSpan.setAttribute( | ||
| ApiTsAttributes.API_TS_ROUTE_PATH, | ||
| req.httpRoute.path || '', | ||
| ApiTsAttributes?.API_TS_ROUTE_PATH || '', | ||
| req.httpRoute?.path || '', | ||
| ); | ||
| encodeSpan.setAttribute( | ||
| ApiTsAttributes.API_TS_ROUTE_METHOD, | ||
| req.httpRoute.method || '', | ||
| ApiTsAttributes?.API_TS_ROUTE_METHOD || '', | ||
| req.httpRoute?.method || '', | ||
| ); |
| req.httpRoute.path || '', | ||
| ); | ||
| encodeSpan.setAttribute( | ||
| ApiTsAttributes.API_TS_ROUTE_METHOD, |
Check failure
Code scanning / CodeQL
Property access on null or undefined Error
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 17 hours ago
To fix the issue, we need to ensure that req.httpRoute is defined before attempting to access its method property. This can be achieved by adding a conditional check to verify the existence of req.httpRoute. If req.httpRoute is null or undefined, we should avoid accessing its properties and handle the situation gracefully (e.g., by setting a default value for the attribute).
The best way to fix the problem is to modify the code on line 149 to include a check for req.httpRoute. If req.httpRoute is not defined, we can set the attribute ApiTsAttributes.API_TS_ROUTE_METHOD to an empty string or another appropriate default value.
| @@ -149,3 +149,3 @@ | ||
| ApiTsAttributes.API_TS_ROUTE_METHOD, | ||
| req.httpRoute.method || '', | ||
| req.httpRoute ? req.httpRoute.method || '' : '', | ||
| ); |
| return result; | ||
| } catch (error) { | ||
| // Record error | ||
| handleGenericError(encodeSpan, error); |
Check failure
Code scanning / CodeQL
Invocation of non-function Error
Copilot Autofix
AI about 17 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| }; | ||
|
|
||
| // Mark as patched | ||
| (patched as any)[kPatched] = true; |
Check warning
Code scanning / CodeQL
Implicit operand conversion Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 17 hours ago
To fix the issue, we need to ensure that kPatched is explicitly defined and initialized before it is used as a property key. This can be achieved by importing or defining kPatched properly in the file. If kPatched is meant to be a string constant, it should be explicitly assigned a value. If it is imported from another module, we need to verify that the import statement is correct and that the module provides a valid value.
| @@ -10,3 +10,4 @@ | ||
|
|
||
| import { TypedExpressRouterInstrumentationConfig, kPatched } from './types'; | ||
| import { TypedExpressRouterInstrumentationConfig } from './types'; | ||
| const kPatched = '__patched'; // Explicitly define kPatched as a string constant | ||
| const { version } = require('../package.json'); |
| }); | ||
|
|
||
| app.get('/post/:id', (req, res) => { | ||
| res.send(`Post id: ${req.params.id}`); |
Check failure
Code scanning / CodeQL
Reflected cross-site scripting High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 17 hours ago
To fix the reflected XSS vulnerability, the user-controlled input (req.params.id) must be sanitized or escaped before being incorporated into the HTTP response. The best approach is to use a library like escape-html to ensure proper encoding of special characters, preventing malicious scripts from being executed in the browser.
Steps to fix:
- Import the
escape-htmllibrary at the top of the file. - Use the
escapefunction fromescape-htmlto sanitizereq.params.idbefore embedding it in the response.
Changes required:
- Add the
escape-htmllibrary import. - Modify the response on line 47 to use
escape(req.params.id).
| @@ -33,2 +33,3 @@ | ||
| import * as http from 'http'; | ||
| import escape from 'escape-html'; | ||
|
|
||
| @@ -46,3 +47,3 @@ | ||
| app.get('/post/:id', (req, res) => { | ||
| res.send(`Post id: ${req.params.id}`); | ||
| res.send(`Post id: ${escape(req.params.id)}`); | ||
| }); |
| @@ -67,3 +67,4 @@ | ||
| "mocha": "^10.8.2", | ||
| "ts-node": "^10.9.2" | ||
| "ts-node": "^10.9.2", | ||
| "escape-html": "^1.0.3" | ||
| }, |
| Package | Version | Security advisories |
| escape-html (npm) | 1.0.3 | None |
| app.use('/double-slashes/', router); | ||
| router.get('/:id', (req, res) => { | ||
| setImmediate(() => { | ||
| res.status(200).end(req.params.id); |
Check failure
Code scanning / CodeQL
Reflected cross-site scripting High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 17 hours ago
To fix the reflected cross-site scripting vulnerability, the user-controlled input (req.params.id) must be sanitized or escaped before being written to the HTTP response. The best approach is to use a library like escape-html, which provides contextual escaping for HTML content. This ensures that any special characters in the input are properly encoded, preventing malicious scripts from being executed.
Steps to fix:
- Import the
escape-htmllibrary at the top of the file. - Replace the direct usage of
req.params.idin the response withescape(req.params.id)to sanitize the input.
| @@ -70,4 +70,5 @@ | ||
| router.get('/:id', (req, res) => { | ||
| const escape = require('escape-html'); | ||
| setImmediate(() => { | ||
| res.status(200).end(req.params.id); | ||
| res.status(200).end(escape(req.params.id)); | ||
| }); |
| @@ -67,3 +67,4 @@ | ||
| "mocha": "^10.8.2", | ||
| "ts-node": "^10.9.2" | ||
| "ts-node": "^10.9.2", | ||
| "escape-html": "^1.0.3" | ||
| }, |
| Package | Version | Security advisories |
| escape-html (npm) | 1.0.3 | None |
Ticket: DX-1473
This PR implements an opentelemetry instrumentation for @api-ts/typed-express-router.
It patches the library's
wrapRouterandcreateRouterfunctions and adds middleware to each request method.To use this instrumentation, register it with the otel sdk.